Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat:User CRUD #35

Merged
merged 9 commits into from
Apr 10, 2023
Merged

feat:User CRUD #35

merged 9 commits into from
Apr 10, 2023

Conversation

ergjustin
Copy link
Contributor

@ergjustin ergjustin commented Mar 7, 2023

Related Issues:

USERS CRUD
Resolves #30 #36 #37

Main Changes:

  • Users list
  • User create || if person without same email create new person with given data
  • User update
  • User delete

Steps To Test:

  1. Login
  2. Navigate to http://localhost:3000/users
  3. Check users list, create, update and delete

@ergjustin ergjustin requested a review from devincowan March 7, 2023 15:41
Copy link
Collaborator

@devincowan devincowan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ergjustin nice work!
A few questions for you, but overall this looks great

@devincowan devincowan linked an issue Mar 7, 2023 that may be closed by this pull request
Copy link
Collaborator

@devincowan devincowan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@ergjustin
Copy link
Contributor Author

Related Issues: USER CRUD

Except password reset everything else finished.

@ergjustin ergjustin requested a review from devincowan March 20, 2023 20:32
Copy link
Collaborator

@devincowan devincowan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all of the updates @ergjustin!
A few issues that we can discuss on our call

header: 'Admin section',
items: [
{
can: ['read', 'users'],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

admin section will still be shown to a standard user in this case.
I believe admin section should be shown to only manager and sys admin
Related to #43

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making a note that we also want this nav section to be hidden when you "drill down" into an Org. And we want to change the name Admin Section => Admin

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

admin section will still be shown to standard users (in addition to managers and admins) but perhaps that is our intent

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we discussed today, @ergjustin will make this can -> canCreate

@ergjustin ergjustin requested a review from devincowan March 24, 2023 16:20
Copy link
Collaborator

@devincowan devincowan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking really good, couple questions...

@ergjustin ergjustin requested a review from devincowan March 27, 2023 18:13
Copy link
Collaborator

@devincowan devincowan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good @ergjustin!

header: 'Admin section',
items: [
{
can: ['read', 'users'],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

admin section will still be shown to standard users (in addition to managers and admins) but perhaps that is our intent

@ergjustin ergjustin requested a review from devincowan April 4, 2023 22:50
Copy link
Collaborator

@devincowan devincowan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @ergjustin!

@devincowan
Copy link
Collaborator

I think we are finally good to merge this unless there are other pending issues I'm not aware of

@devincowan devincowan merged commit 3b6e8f9 into feat-cuahsi-mgmt Apr 10, 2023
@devincowan devincowan deleted the dev-justin branch April 10, 2023 18:03
This was referenced Apr 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

password complexity check + confirm password match user: name vs full_name differentiation users CRUD
2 participants